Less overzealous inlining #183
Conversation
|
I'm not sure if it's intended like this, based on the description: @DJMcNab probably knows more. |
DJMcNab
left a comment
DJMcNab
left a comment
There was a problem hiding this comment.
I agree that the inline attributes are currently overzealous. I believe however that this PR goes too far the other way.
I think that it's also possible that the inner functions should be more properly marked as inline(never) (or even that we should provide two versions of vectorize, one which is unhinted and one which inline(never))
I want to see what Valerie thinks here.
| fn level(self) -> Level { | ||
| Level::Avx2(self) | ||
| } | ||
| #[inline] |
There was a problem hiding this comment.
This function should still be #[inline] (indeed, I believe that it possibly should be inline(always)). This is because we don't want to force LLVM to change its calling convention to call this function, we just want to break unneeded inlining.
| fn level(self) -> Level { | ||
| Level::Neon(self) | ||
| } | ||
| #[inline] |
| Level::baseline() | ||
| } | ||
| } | ||
| #[inline] |
| #[cfg(not(feature = "force_support_fallback"))] | ||
| Level::baseline() | ||
| } | ||
| #[inline] |
| fn level(self) -> Level { | ||
| Level::WasmSimd128(self) | ||
| } | ||
| #[inline] |
|
Thanks! That makes sense, I'll try to update this PR with those changes (no I don't think a second |
|
I've updated the PR to only remove |
3 participants
Using
vectorize()as opposed to directly calling functions annotated#[inline(always)]is meant to introduce an inlining barrier and make the compiler make inlining decisions. Therefore putting#[inline]on it is rather pointless: if I wanted aggressive inlining, I wouldn't be callingvectorize()in the first place.#[inline]also enables cross-crate inlining, but sincevectorize()is a generic function it will be instantiated in the caller crate, so this is not a concern.More info about inlining: https://matklad.github.io/2021/07/09/inline-in-rust.html
Motivation
I'm seeing a large performance regression from switching fromIt's not clear if over-zealous inlining was a cause of the regression, there are other factors. I'll re-measure once #185 lands.widetofearless_simdin QuState/PhastFT#58 due to overly aggressive inlining. This alone doesn't fix it, but seems like a sensible first step.